Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raytrace plots #2655

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Raytrace plots #2655

wants to merge 10 commits into from

Conversation

gridley
Copy link
Contributor

@gridley gridley commented Aug 18, 2023

Description

This PR implements 3D plotting with Phong shading. I factored out the basic ray creation from a camera into an abstract base class called RayTracePlot. Then, ProjectionPlot implements the logic for moving rays all the way through a geometry in order to wireframe cells or materials of interest and color them in an x-ray-like view. The new PhongPlot (happy to rename) lets the user specify a few materials or cells which they would like to have be opaque, with everything else in the model being transparent. By default it assumes that a light is placed at the camera's location, but the light can optionally be moved.

As an example, here is a reminder of what a ProjectionPlot looks like:

orthographic_example1

A Phong plot on a similar geometry looks kinda neat, and could certainly be a helpful tool for visualization on intricately structured stuff--especially for communicating what a model looks like to other people.

phong

You can adjust the fraction of diffuse light to give a slightly different look. By default, it uses 10% diffuse lighting, 90% lighting from the light source with single scattered isotropic scattering off surfaces.

phong_diffuse

You can move the light source location, which can make a nice dramatic effect:

phong_move_light

I was frustrated that we have to create a full Particle object just for geometric operations. As a result, I factored out the geometry state of Particle into a base class called Geometron. This comes in handy for openmc_find_cell as we no longer have to initialize the cross section caches and all that other physics cache data just to do a cell search. Similarly the Geometron is instead used for ray tracing in the plotter, now. It could also be used for the implementation of the random ray method which I hear is coming along. My last note on the Geometron is that all the geometric operations take Geometron references or pointers, not Particle. Of course, simply passing a Particle pointer works as usual. This does not invoke any polymorphism, so there will be zero performance overhead as a result of this, or perhaps even performance gains since the compiler will know that only a smaller subset of the data can potentially be accessed. Polymorphism is used for error handling in geometric routines. Geometrons fatal_error on erroneous leakage from the model, but Particles save the leaked ID and then the simulation continues.

In this direction, there was one larger change I had to make for error handling in geometry. It seems that raising an exception is the right thing to do here, with calling code defining the exception handling. Code in Particle now will resort to mark_as_lost for lost stuff, but in the plotter, you just get a plain old exception that stops the program. Previously it would just say "particle with ID=-1 leaked" or something like that. For MOC or other things like that, of course you would just want the program to exit if you leak a ray, so I hope other people like this error handling approach in the geometry now. Also, the compiler knows that exceptions happen with low frequency, so there might be a slight performance gain from the improved branch prediction in those few places where we previously had just called mark_as_lost.

Lastly, in the ParticleData class, I moved the comments to the setters/getters rather than on the private variables, since this is the public-facing API that people would actually want to read.

There are a few other things I want to do before merging this. It is a WIP. Of course I need to add the new plot and its options to the python API. Really not looking forward to the XML boilerplate. Also I want to factor the plot ray tracer into its own RayKernel-object. I'm thinking it'd be nice to create a ray that just has some lambdas attached to it like on_intersection_ or on_cross_surface_ to avoid repeating it in PhongPlot and ProjectionPlot. Of course this could help with parallelization and could be used in a random ray implementation. Lastly I'd like to add a 3D version of Universe.plot that automatically picks a camera position based on the bounding box size and some simple position argument like "top left" or "front left".

Also, I need to figure out how to rotate surface normals in transformed nested universes in order to get the direction to the light in the root universe. That's going to be a fun one.

Oh wait... there's one more thing. The ProjectionPlot camera rays were not uniform in pixel space, which did not create distortion effects for things that are far away, but are distorted for things up close. The old plotter would make straight surfaces appear curved, e.g:

example1

With this PR, it correctly maps pixels onto ray directions, making it look a lot better:

example1

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format on any C++ source files (if applicable)
  • I have followed the style guidelines for Python source files (if applicable)
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • Add PhongPlot to python API
  • Factor out ray tracing logic into RayTracePlot (it is duplicated right now)
  • Add Universe plot3d method for easily obtained 3D visualization in ipython notebooks
  • Plot a slice of the BEAVRS core with pins and core structure shown as solids.
  • Make it work with rotated/translated subuniverses

@gridley gridley added the Plots Changes related to plotting of models and results label Aug 18, 2023
@gridley
Copy link
Contributor Author

gridley commented Aug 26, 2023

Just sharing some new stuff. Here's a plot of some TRISO:

image

And here's an example of shadows working correctly with the classic CSG test object:

csg_background

Copy link
Contributor

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very excited about this. Between the projection plots and this new capability we're building up to a fairly advanced visualization capability! Thanks for driving this effort @gridley!

This is a first-round review. I think we're at a place where we want to make sure we're really happy with the design so we can extend it further. I'm going to try this out with some DAGMC geometries too just to see how far I can get.

include/openmc/geometry.h Outdated Show resolved Hide resolved
include/openmc/geometry.h Outdated Show resolved Hide resolved
include/openmc/particle_data.h Outdated Show resolved Hide resolved
include/openmc/plot.h Show resolved Hide resolved
include/openmc/geometry.h Outdated Show resolved Hide resolved
src/plot.cpp Outdated Show resolved Hide resolved
src/plot.cpp Outdated Show resolved Hide resolved
src/plot.cpp Outdated Show resolved Hide resolved
src/plot.cpp Outdated Show resolved Hide resolved
src/plot.cpp Outdated Show resolved Hide resolved
include/openmc/dagmc.h Outdated Show resolved Hide resolved
@shimwell
Copy link
Member

Just made one of these plots, very nice interface and easy to use.
p

@gridley
Copy link
Contributor Author

gridley commented Sep 9, 2023

""Some files failed the formatting check! See job summary and file annotations for more info" && exit 1"

How can I look at these annotations? Huh. Thought I was running clang-format...

@pshriwise
Copy link
Contributor

""Some files failed the formatting check! See job summary and file annotations for more info" && exit 1"

You can see them in the run summary by clicking on the 🏠 icon in the left sidebar
https://github.com/openmc-dev/openmc/actions/runs/6133354411?pr=2655

How can I look at these annotations? Huh. Thought I was running clang-format...

Are you running the same version as CI (15)? We've seen a few others get hit by that recently.

@gridley
Copy link
Contributor Author

gridley commented Oct 5, 2023

We should be good for another review here, now! I removed the exception crap to just use mark_as_lost and fatal_error instead. However, we need to use a virtual here in order to retain the correct behavior: die with an error for Geometron, but save the particle info and continue for Particle.

@pshriwise
Copy link
Contributor

I think I still owe you a mini-PR to handle DAGMC geometry. I'll try to take care of that today. Hopefully I can review it soon.

@gridley
Copy link
Contributor Author

gridley commented Oct 5, 2023

Oh sweet! OK I just realized that the mark_as_lost does actually have to be virtual to get the right behavior, and I have to move the id_ data to Geometron, but that's not really a big deal. (so gonna push that ina sec)

@gridley
Copy link
Contributor Author

gridley commented Oct 5, 2023

Alright, now it should be good for review!

Copy link
Contributor

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments from a mini-review. DAGMC PR imminent once I rebase my test branch onto this....

src/plot.cpp Outdated Show resolved Hide resolved
src/plot.cpp Show resolved Hide resolved
src/plot.cpp Outdated Show resolved Hide resolved
@gridley
Copy link
Contributor Author

gridley commented Oct 7, 2023

Just sharing this nice image I made for the docs. Imagine how many voxels you'd need to create this previously!
phong_triso

@gridley
Copy link
Contributor Author

gridley commented Oct 7, 2023

@cfichtlscherer has some super nice stuff too. Are you able to give any sneak peeks Chris?

@gridley
Copy link
Contributor Author

gridley commented Oct 10, 2023

test_evaluate_legendre is failing for me in CI. No way it's related to the recent commits... Any ideas?

@pshriwise
Copy link
Contributor

pshriwise commented Oct 10, 2023

test_evaluate_legendre is failing for me in CI. No way it's related to the recent commits... Any ideas?

I've been seeing that too here: https://github.com/openmc-dev/openmc/actions/runs/6471480840/job/17575480291

I'll take a look soon.

@cfichtlscherer
Copy link
Contributor

@cfichtlscherer has some super nice stuff too. Are you able to give any sneak peeks Chris?

Yeah I used it to plot some weapon models. I think it would be so nice to have it in the main code.

warheads_models_01102023

void Ray::trace()
{
int first_surface_ = -1; // surface first passed when entering the model
first_inside_model_ = true; // false after entering the model
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would performing a exhaustive_find_cell check to start and calling RayTracePlot::advance_to_boundary_from_void if the result is false allow us to simplify this loop a bit?

Speaking of RayTracePlot::advance_to_boundary_from_void, is there any reason it isn't a method of the Ray class itself?

@shimwell
Copy link
Member

wondering if metallic or surface texture or reflective surfaces would be possible

// We cannot detect it in the outer loop, and it only matters here, so
// that's why the error handling is a little different than for a lost
// ray.
if (i_surface() == -1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be another issue that could be solved by using our particle transport mechanisms for surface intersection and crossing.

@pshriwise
Copy link
Contributor

Hey @gridley. Sorry it's taken me a while to get back to you. I talked with @jtramm about this PR and what overlap there might be with his random ray implementation. For the time being, we settled on separate implementations b/c, from what we could tell, there wouldn't be a lot of overlap aside from the definition of the ray itself (position and direction). We also identified a couple of updates here that could be of benefit. I made note of them in file comments.

One overarching note was that the introduction of the Geometron is a great contribution in and of itself. Would you be willing to break that out into a separate PR so we can get that merged more quickly? I think it would speed up review iteration and allow us to focus more on the plotting aspects of the work only.

@pshriwise
Copy link
Contributor

test_evaluate_legendre is failing for me in CI. No way it's related to the recent commits... Any ideas?

This has been resolved in #2729. We'll need to update this branch to get CI passing again.

@gridley gridley mentioned this pull request Oct 22, 2023
5 tasks
@gridley gridley force-pushed the raytrace_plots branch 2 times, most recently from f58d45e to d8fc057 Compare February 25, 2024 17:24
@gridley
Copy link
Contributor Author

gridley commented Feb 25, 2024

OK @pshriwise, just resolved any conflicts, could you give this one more review?

@pshriwise
Copy link
Contributor

OK @pshriwise, just resolved any conflicts, could you give this one more review?

Can do! Thanks for updating it

Copy link
Contributor

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as awesome as the first time I looked at it! I had a good time playing around with it today.

image

image

Making note of some future work for now:

  • focal plane setting
  • pixel position generator classes for perspective and orthographic projections
  • a camera object
  • create a GeometryState::advance method
  • add a method for line segment entries based on plot_.color_by to reduce code duplication

I wouldn't say any of these are blocking, aside perhaps from the inverse_rotation line comment, but I wanted to write them down while they're in my head.

include/openmc/plot.h Outdated Show resolved Hide resolved
include/openmc/plot.h Outdated Show resolved Hide resolved
include/openmc/plot.h Outdated Show resolved Hide resolved
include/openmc/plot.h Outdated Show resolved Hide resolved
src/plot.cpp Show resolved Hide resolved
openmc/plots.py Outdated Show resolved Hide resolved
openmc/plots.py Outdated Show resolved Hide resolved
openmc/plots.py Outdated Show resolved Hide resolved
openmc/plots.py Outdated Show resolved Hide resolved
plots.append(Plot.from_xml_element(e))
elif plot_type == 'slice':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I think this is an improvement, does it break backward compatibility? I can't recall how long the Plot.type attribute has been around...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure it does not, no.

@gridley
Copy link
Contributor Author

gridley commented Mar 7, 2024

OK, pretty sure I got all your comments addressed @pshriwise!

@gridley
Copy link
Contributor Author

gridley commented Mar 7, 2024

This automatic clang format is driving me nuts. I am running clang format, but they're different versions and have some tiny whitespace differences. I've dropped some pretty unnecessary comments in the last commit where I can't figure out how to make the two versions of clang-format happy.

@pshriwise
Copy link
Contributor

This automatic clang format is driving me nuts. I am running clang format, but they're different versions and have some tiny whitespace differences. I've dropped some pretty unnecessary comments in the last commit where I can't figure out how to make the two versions of clang-format happy.

Yeah the C++ formatter check can be pretty frustrating sometimes, sorry. It's plagued others as well. It would be great if we could configure a bot to provide the updates as suggestions or something. I'll give that some thought.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Plots Changes related to plotting of models and results
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants